Add muldiv_c and muxadd peepopts
#4740
Conversation
|
Thanks for the feedback on |
Co-authored-by: Martin Povišer <povik@cutebit.org>
Co-authored-by: Martin Povišer <povik@cutebit.org>
Compiles and transforms correctly, fails equiv
Fix code review issue
…ng on same mux-add pair
| bool c_const_signed = div->getParam(ID::B_SIGNED).as_bool(); | ||
| int b_const_int = b_const.as_int(b_const_signed); | ||
| int c_const_int = c_const.as_int(c_const_signed); | ||
| int b_const_int_shifted = b_const_int << offset; |
There was a problem hiding this comment.
The as_int() conversion may overflow if the operands are too wide. We should catch this case and bail
There was a problem hiding this comment.
I think b_const_int << offset can still overflow
There was a problem hiding this comment.
Not sure how to proceed, a test specifically designed for this condition will help.
There was a problem hiding this comment.
As long as you test b_const_int.size() + offset <= 31 you should be fine
passes/pmgen/peepopt_muldiv_c.pmg
Outdated
| } | ||
|
|
||
| // Rewire to only keep multiplier | ||
| mul->setPort(\B, Const(b_const_int_shifted / c_const_int, b_const_width)); |
There was a problem hiding this comment.
Because b_const_int_shifted is shifted up, there's no guarantee it fits into b_const_width
Also this might need special care wrt signedness. B_SIGNED on the multiplier may be false but b_const_int_shifted / c_const_int can be negative due to the divider. I wouldn't be opposed to requiring matching signedness on the divider and multiplier to make the transformation a little easier to reason about
There was a problem hiding this comment.
@povik should I change to: mul->setPort(\B, Const(b_const_int_shifted / c_const_int, b_const_width + offset));
There was a problem hiding this comment.
That would work but I suggest constructing the Const with 32 bits (the constructor default) and then calling compress(/signedness/) to fit to the constant. The reason for this is: unless wreduce is sequenced after peepopt I am not sure any pass would shrink the operand before we bitblast the multiplier
There was a problem hiding this comment.
Implemented here: akashlevy@8b08f81
and
akashlevy@8687e5f
|
I've added an implementation of muldiv_c tests approximately as specified, see comments in the spec doc. Please pull and resolve the failures and feel free to ask for more context |
Passing equiv for simplest muxadd case, prevent multiple match/rewiri…
Switch formal proof to use miter/sat
Convert to miter/sat
muxadd pass basic equiv_opt
log test header for debug
Fix symmetry test
|
@povik @georgerennie @widlarizer Can we split each test into a different file so that we can run each test individually? Right now, the script stops every time a test fails, so we can't easily tell how many tests are passing/failing. Additionally, now that we have a (somewhat) workable pass, it would be great if you can analyze any test failures and ensure that the test is not the problem. |
Pass all muldiv tests
|
It's a little bit more convenient to keep it in one file for long term maintanance. These are regression tests so they're meant to all pass in sequence and stopping on the first failure is fine. For development you could copy and chop them up and create a temporary directory with them that isn't tracked or is temporarily tracked under version control |
|
The only failing case I see is a missed optimization: |
|
@widlarizer I made a fix here: akashlevy#19, but I also have to increase one constant to actually overflow. Since we are looking at the actual value, not the declared size: 4'd4 only takes 3 bits, 4'd8 takes 4 bits and indeed overflow and get caught. |
|
@widlarizer @povik @georgerennie I just rebased. All of the tests appear to be passing, and I think we have addressed all your review comments. Can you confirm that everything looks ok on your end now? |
Fix nanoxplore meminit test
muxadd breaks Quicklogic dsp inference, make it optional
|
Note: |
What are the reasons/motivation for this change?
This PR adds two peep-hole optimizations that improve netlist quality:
muldiv_c:y = (a * b_const) / c_const ===> a * eval(b_const / c_const)ifb_const % c_const == 0. This implements basic constant propagation for multipliers and dividers with divisible constants.muxadd:y = s ? (a + b) : a ===> y = a + (s ? b : 0). This provides useful restructuring for improving logic optimization.s ? b : 0can be optimized to a bitwise AND in subsequent optimizations.Explain how this is achieved.
passes/pmgen/peepopt_muldiv_c.pmg:muldiv_cpeepoptpasses/pmgen/peepopt_muxadd.pmg:muxaddpeepoptpasses/pmgen/peepopt.cc: Add to docs and run peepopts duringpeepoptpasspasses/pmgen/Makefile.inc: Add the new sourcesIf applicable, please suggest to reviewers how they can test the change.
muxaddmuldiv_cmuxaddmuldiv_cmuxaddmuldiv_cmuxaddmuldiv_cmuxaddmuldiv_cequiv_opt,miter, or FOSSeqymuxaddmuldiv_c